fix: correct the url for flag images [Fixes #172] #173
Conversation
|
| @@ -82,15 +84,15 @@ const PhoneNumberForm = ({ | |||
| > | |||
| <div width="30px">+ {code}</div> | |||
| <img | |||
| width="40px" | |||
| width="28px" | |||
There was a problem hiding this comment.
Why has this been changed ?
There was a problem hiding this comment.
40px looked really huge. have changed it back.
| style={{ | ||
| marginLeft: 8, | ||
| marginRight: 8, | ||
| }} | ||
| loading="lazy" | ||
| src={`/assets/flags/${dialCodeCountryCodeMap[ | ||
| src={getFlagOfCountry(dialCodeCountryCodeMap[ |
There was a problem hiding this comment.
Test it properly, getting below error while testing:
Uncaught i18nify Error: Error: Invalid country code: AC
There was a problem hiding this comment.
this is failing because flags are missing for the three country code(AC,XK,001 mentioned in phoneNumber.js in dialCodeMap and dialCodeCountryCodeMap. Need to add it in https://unpkg.com/@razorpay/[email protected]/lib/assets/flags/ac.svg
There was a problem hiding this comment.
if you comment these flags in dialCodeMap and dialCodeCountryCodeMap in phoneNumber.js. No error will be there
There was a problem hiding this comment.
I have stored the path in constants and using the variable to make the url. This way we won't get an error. Have tested the same. Please check now. @RgnDunes Thanks!
|
@RgnDunes No more changes are needed from the code point of view. If you comment these flags in dialCodeMap and dialCodeCountryCodeMap in phoneNumber.js. No error will be there. These flags are also missing in the i18nify-data/assets/flags folder. |
@sarthak-kumar-shailendra dialCodeCountryCodeMap, dialCodeMap is being used heavily by Phone Number module, removing something might break some actions or apis might not work for certain geo's. This approach is not recommended. Please add the flags instead. |
It was failing because of the country code 001 which is of US. Have handled this case in img tag and same is also the case with ascension island. Please check now @RgnDunes |
sarthak-kumar-shailendra
force-pushed
the
master
branch
from
4d5ddbb to
87147b1
Compare
|
Please review @RgnDunes |
|
@varunkumark1997 @RgnDunes @rzp-Piyush can you guys please review this? |
![https://unpkg.com/@razorpay/[email protected]/lib/assets/flags/ac.svg](https://unpkg.com/@razorpay/i18nify-js@1.12.2/lib/assets/flags/ac.svg){kind=link}
sarthak-kumar-shailendra
force-pushed
the
master
branch
from
87147b1 to
e347a9c
Compare
Description
Flag Images URL is not properly set in the phone number menu. [#172]
As a result of which images are not loaded.
We already have getFlagOfCountry module in i18nify-js for loading the images from the cdn. And we are using the same in other components as well like https://geosmart.razorpay.com/#/geo/getFlagOfCountry. I am also using the same module to set the url in the image tag.
Changes Made
List the main changes made in this pull request.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Additional Notes
Any additional information that would be helpful for the reviewer.
Checklist:
Reviewer Checklist
PR Title Format
Format:
<type>: <subject>Types can be as follows:
feat: (new feature for the user, not a new feature for build script)fix: (bug fix for the user, not a fix to a build script)docs: (changes to the documentation)style: (formatting, missing semi colons, etc; no production code change)refactor: (refactoring production code, eg. renaming a variable)test: (adding missing tests, refactoring tests; no production code change)chore: (updating grunt tasks etc; no production code change)